Skip to content

Bound lines array in getLastNVisualLines to prevent unbounded memory growth - #1220

Open
nordicnode wants to merge 1 commit into
CodebuffAI:mainfrom
nordicnode:perf-visual-lines-bounded-buffer
Open

Bound lines array in getLastNVisualLines to prevent unbounded memory growth#1220
nordicnode wants to merge 1 commit into
CodebuffAI:mainfrom
nordicnode:perf-visual-lines-bounded-buffer

Conversation

@nordicnode

Copy link
Copy Markdown

Bound lines array in getLastNVisualLines to prevent unbounded memory growth

Summary

• In cli/src/utils/text-layout.ts, bound the lines accumulator in getLastNVisualLines to at most n lines.
• Previously, getLastNVisualLines accumulated every wrapped line from character 0 to the end of the text in lines: string[], only to slice lines.slice(-n) at the very end and discard the rest. When rendering long reasoning blocks, assistant messages, or terminal logs (e.g. 500+ lines), this held hundreds of unnecessary line strings in memory and required an additional .slice() array allocation.
• Updated pushLine() to evict the oldest line whenever lines.length === n and set hasMore = true, ensuring memory used by lines is strictly bounded by $O(n)$ ($n \le 5$ in preview) rather than $O(\text{total lines})$.
• Added comprehensive unit tests in cli/src/utils/__tests__/text-layout.test.ts covering zero column/line inputs, small line counts, wrapping boundaries, and multi-hundred line inputs.

Test plan

[✓] bun test --config=/dev/null src/utils/__tests__/text-layout.test.ts — 11 pass, 0 fail
[✓] bun run --cwd cli typecheck — 0 errors
[✓] PR hygiene check passed

@codebuff-team

Copy link
Copy Markdown
Contributor

Nice catch and clean fix. The original getLastNVisualLines accumulated every wrapped line for the full text just to slice(-n) at the end, which is wasteful for long text with a small n (per the code, n <= 5). Your pushLine eviction (shift-then-push once lines.length === n) keeps the accumulator bounded and produces the same output.

A couple of things worth double-checking before this lands:

  1. Array.prototype.shift() is O(current length) per call. Since n is small in practice this is negligible, but a small ring buffer (fixed-size array + head index) would avoid the shift cost entirely and be a more idiomatic 'bounded queue' if this function is ever called with a larger n. Not blocking, just a note for future-proofing.
  2. The trailing-push condition changed from current.length > 0 || lines.length === 0 to current.length > 0 || (!hasMore && lines.length === 0). This looks correct given n > 0 is enforced upstream (lines never drain back to zero once eviction begins), but it's a subtle enough change that it deserves a one-line comment explaining why hasMore is checked here, for the next person reading it.

Test coverage is solid — zero/negative bounds, exact-n, over-n with eviction, and long-line wrapping are all exercised, and they pass. This is a small, self-contained, well-motivated change that's easy to port by hand. Good first PR.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants